fix(dsn): skip non-regular env files during detection#806
Merged
BYK merged 2 commits intogetsentry:mainfrom Apr 22, 2026
Merged
fix(dsn): skip non-regular env files during detection#806BYK merged 2 commits intogetsentry:mainfrom
BYK merged 2 commits intogetsentry:mainfrom
Conversation
Member
|
Thanks so much! I'm refactoring file walkers so holding off for just a bit before I land that and rebase/refactor your patch on top of it |
BYK
added a commit
that referenced
this pull request
Apr 22, 2026
…#813) ## Summary - Three failures on [PR #806](#806) / [run 24747631157](https://github.com/getsentry/cli/actions/runs/24747631157) traced to fork-PR restrictions: 1. `check-generated` checks out `github.head_ref` from the base repo, but that branch only exists on the fork → `git fetch` exits 1. 2. `build-binary` and `build-npm` hard-fail because `${{ vars.SENTRY_CLIENT_ID }}` isn't reaching fork jobs (likely blocked by the `getsentry` org's "Send variables to fork PR workflows" policy). 3. `CI Status` fails because of (1) and (2). ## Fix - **`check-generated` checkout**: condition `ref:` on whether the app token step succeeded. Same-repo PRs keep the branch-head checkout (so auto-commit can `git push` regenerated docs back). Fork PRs leave `ref` empty → `actions/checkout` defaults to `GITHUB_REF` (the PR merge SHA, always fetchable from the base repo with `github.token`). - **`build-binary` / `build-npm` env**: fall back to a dummy `SENTRY_CLIENT_ID` when `vars.SENTRY_CLIENT_ID` is empty. All tests in the repo already tolerate any non-empty value (see `test/preload.ts` and the `"test-client-id"` defaults in e2e tests); CI smoke tests are `--help` only. PR binaries never ship, so runtime OAuth paths aren't exercised. ## What doesn't change - Same-repo PRs and `main`/`release/**` pushes still bake the real `vars.SENTRY_CLIENT_ID` into the binary. - `script/build.ts` and `script/bundle.ts` keep their hard-fail guard — useful dev ergonomic for local builds. - Security posture: no `pull_request_target`, no new secret exposure, no org-policy change. ## Test plan - Own-repo CI on this PR must pass (baseline sanity). - After merge, ask @elucid to rebase #806 to validate the fork path.
927877d to
168205e
Compare
BYK
added a commit
that referenced
this pull request
Apr 22, 2026
## Summary Follow-up to #813. The `Docs Preview` workflow (`docs-preview.yml`) tries to push to the base repo's `gh-pages` branch, but fork PRs get a read-only `GITHUB_TOKEN` and fail with HTTP 403 (`Permission to getsentry/cli.git denied to github-actions[bot]`). Observed on [PR #806 / run 24776423153](https://github.com/getsentry/cli/actions/runs/24776423153/job/72495750855?pr=806) after rebasing onto main. ## Fix Gate the `.nojekyll` setup step and the `Deploy Preview` step with `if: github.event.pull_request.head.repo.full_name == github.repository || github.event_name != 'pull_request'`. The build step still runs unconditionally so docs compilation errors surface on fork PRs — just no live preview published. ## Test plan - Same-repo PRs / pushes to main: unchanged, preview still deploys. - Fork PR #806: re-run CI after this merges; `preview` job should succeed (build + skip deploy).
DSN auto-detection reads .env* files to infer SENTRY_DSN and project context. When one of those paths is a FIFO (including 1Password's symlink-to-FIFO env files), Bun.file().text() can block indefinitely waiting for a writer. Stat each .env* path before reading and skip non-regular files during: - explicit env-file scanning - project-root walk-up checks - cached DSN source verification This intentionally avoids the hang rather than trying to consume FIFO contents. Regular files and symlinks to regular files still work as before. Also add regression coverage for top-level detectDsn() and the cached verification path when an env file becomes a symlinked FIFO.
168205e to
9cb0d33
Compare
6 tasks
BYK
added a commit
that referenced
this pull request
Apr 22, 2026
## Summary Extends the FIFO-safety pattern from #806 beyond DSN detection. All remaining single-file reads of user-controlled paths now have a `stat.isFile()` guard (either via a new `safeReadFile` helper or inline), eliminating the FIFO-hang bug class across the codebase. ## Why PR #806 fixed `Bun.file().text()` hanging on 1Password-style symlinked FIFOs for DSN detection (4 sites in `src/lib/dsn/`). But the CLI has ~4 more read sites under user-controlled paths with the same vulnerability: - **`src/lib/sentryclirc.ts`** — reads `.sentryclirc` at every directory during walk-up (hit on every CLI invocation). - **`src/lib/init/tools/read-files.ts`** — LLM-driven reads of arbitrary project files during init wizard. - **`src/lib/init/tools/apply-patchset.ts`** — reads modify-target files during init wizard. - **`src/lib/init/workflow-inputs.ts::preReadCommonFiles`** — scans a fixed allowlist of common config filenames. All four would hang on a 1Password-symlinked `.env`, `.sentryclirc`, `package.json`, etc. An audit pass also surfaced a subtle bug in the two sites that already had a `stat()`-based size check: `stat` on a FIFO returns successfully with `size=0`, so the size-check passes and the subsequent read still hangs. Adding `stat.isFile()` alongside the size check fixes this without a second `stat` call. ## Design Two distinct policies, corresponding to two different read contexts: ### `src/lib/safe-read.ts::safeReadFile` — opportunistic best-effort reads ```ts safeReadFile(path, operation) → Promise<string | null> ``` Chains `isRegularFile` (stat + FIFO guard) with `Bun.file(path).text()`. Returns `null` for any expected error (ENOENT, EACCES, EPERM, EISDIR, ENOTDIR, non-regular file, etc.); unexpected errors route through the existing `handleFileError` → Sentry pipeline and also return `null`. Used by `apply-patchset.ts` (where null is translated to a targeted throw). ### `sentryclirc.ts::tryReadSentryCliRc` — committed config load Direct `fs.promises.stat` + narrow `try/catch` that returns `null` only on ENOENT/EACCES; re-throws every other error. A user with a broken `.sentryclirc` (EPERM at the dir level, EISDIR, EIO, etc.) sees the error clearly rather than getting confusing downstream "no auth token" failures. The two sites with size-gated reads (`read-files.ts`, `workflow-inputs.ts::preReadCommonFiles`) keep their existing `fs.promises.stat` call but now check `stat.isFile()` alongside `stat.size`. No extra stat calls. ## Per-site changes | Site | Before | After | |---|---|---| | `sentryclirc.ts::tryApplyFile` | Local `tryReadFile` (18 LOC, ENOENT/EACCES-only) | `tryReadSentryCliRc` — adds `stat.isFile()` FIFO guard, keeps the narrow ENOENT/EACCES re-throw policy intact | | `init/tools/read-files.ts` | `fs.promises.stat` + size gate + read | Same flow + explicit `stat.isFile()` check before the read | | `init/tools/apply-patchset.ts` | Direct `fs.promises.readFile(absPath)` | `safeReadFile` → throws targeted "not a regular file or read failed" on null | | `init/workflow-inputs.ts::preReadCommonFiles` | `stat` + size gate + read | Same + `stat.isFile()` check; sets `cache[filePath] = null` on non-regular | ## Regression tests `test/lib/safe-read.test.ts` — 11 tests covering every migration site: - `safeReadFile` on regular file / missing / FIFO / symlink→FIFO / directory - `sentryclirc` loader on a FIFO-backed `.sentryclirc` → empty config, no hang - `readFiles` tool on FIFO and symlink→FIFO paths (both test cases) - `applyPatchset` tool on FIFO and symlink→FIFO targets (both test cases) - `preReadCommonFiles` on a FIFO-backed common-config file **Verified negatively**: for each migrated site, temporarily removing the FIFO guard causes the corresponding regression test to time out at Bun's 5s default, confirming the guards are genuinely the condition being exercised. ## Review cycle The PR went through multiple rounds of self-review + bot review after the initial post. Key fixes from review: - **Seer (HIGH)** flagged that my first pass at migrating sentryclirc silently swallowed EPERM/EISDIR/EIO errors. Fixed by reverting sentryclirc to direct `fs.promises.stat` with the narrow original error policy (not via `isRegularFile`, which had a broader `handleFileError` swallow-list). - **Cursor (Medium)** caught that my second pass *still* had the bug because `isRegularFile` internally calls `handleFileError`. Fixed by calling `stat` directly. - **Cursor (Low)** caught unused re-exports in `safe-read.ts` after the sentryclirc-goes-direct change removed the last consumer. Dropped. - **Cursor (Low)** caught an orphaned JSDoc block after a helper-extraction mid-file. Swapped order. - **Self-review subagent** caught that one test was calling `precomputeDirListing` (a listing op) instead of `preReadCommonFiles` (the function with the new guard). Fixed to call the right function and negative-verified. ## Test plan - [x] `bunx tsc --noEmit` — clean - [x] `bun run lint` — clean (1 pre-existing markdown.ts warning) - [x] `bun test test/lib/safe-read.test.ts` — **11 pass** - [x] `bun test --timeout 15000 test/lib test/commands test/types` — **5687 pass, 0 fail** - [x] `bun test test/isolated` — 138 pass - [x] Negative verification for each guarded site — confirmed genuine regression coverage 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
We use this .env feature from 1password so our .env files are named pipes. Running
sentrycli in directories where this type of file mounted (or symlinked) hangs because it tries to read .env to see if theres aSENTRY_DSNin there. This PR detects this case and skips reading. I figure this is nicer than doing a blocking read on the file to trigger the auth dialog from 1pw.If you'd prefer a different fix, let me know. Just eager to work around this because this new cli is so useful!
Summary
Skip non-regular
.env*files during DSN detection to avoid hangs on FIFO-backed env files (including 1Password symlink-to-FIFO env files).This applies the guard to:
It also adds regression coverage for top-level
detectDsn()and for cached verification when an env file becomes a symlinked FIFO.Testing
bun run testbun run lint(reports one unrelated pre-existing warning insrc/lib/formatters/markdown.ts)